std.child_process: enable non-standard streams #14152

matu3ba · 2023-01-01T23:21:48Z

This PR simplifies the setup code and handling to minimize leaking time
for both Windows and Posix and does not touch the list approach of
Windows. Portable pipes default to pipe2 with CLOEXEC on Posix and
disabled handle inhertence on Windows except shortly before
and after the spawn.

The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.

Leaking on Posix

CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

File leaking on Windows can be fixed with an explicit list approach,
which might require runtime probing to not break on Kernel updates
for the parameter list.

test/standalone/childprocess_extrapipe/parent.zig

lib/std/child_process.zig

matu3ba

fixed todos.

lib/std/os.zig

test/standalone/childprocess_extrapipe/child.zig

test/standalone/childprocess_extrapipe/parent.zig

matu3ba · 2023-01-10T01:32:01Z

lib/std/os.zig

@@ -288,6 +288,27 @@ pub fn close(fd: fd_t) void {
    }
 }

+pub const windowsPtrDigits = 19; // log10(max(usize))


I dont like this solution. These are very frequent operations, so it would be nice to have the sizes next to fd_t on each platform or to have a LUT. Opinions?

matu3ba · 2023-01-10T02:05:38Z

lib/std/os/windows.zig

@@ -1566,6 +1576,45 @@ pub fn GetEnvironmentVariableW(lpName: LPWSTR, lpBuffer: [*]u16, nSize: DWORD) G
    return rc;
 }

+// zig fmt: off


I kept these inside for #14251, but they are generally useful for other users as well short of advanced list attributes.

matu3ba · 2023-01-10T23:48:05Z

fixup naming according to master

~~I feel like this is ready for review once CI is successful. While this is short of #14251 to prevent all leaks on Windows, I think this is very nice to use.~~

Panic handlers spawns itself as child process, such that child communicates passed tests encoded into a message format. Message format: [term of process return, was_panic, message len, optional message] This uses extra stream aka pipe to allow user logging. Depends on ziglang#14152. Closes ziglang#1356.

Panic handlers spawns itself as child process, such that child communicates passed tests encoded into a message format. Message format: [term = process_return, was_panic, message_len, optional message] This uses a non-standard stream=pipe to allow user logging. Depends on ziglang#14152. Closes ziglang#1356.

Panic test runner starts itself as child process with the test function index representing a test block. The custom panic handler compares a global previously written string, which is uable from within a test. If the panic message is expected, the success count is increased and the next test block is run. Failure ouputs the so far collected results before the parent process terminates as well. This means, that only one @Panic is possible within a test block and that no follow-up code after the @Panic in the test block can be run. Depends on ziglang#14152. Closes ziglang#1356.

Panic test runner starts itself as child process with the test function index representing a test block. The custom panic handler compares a global previously written string. If the panic message is expected, the success count is increased and the next test block is run. On failure the parent process prints collected results, before it terminates. This means, that only one @Panic is possible within a test block and that no follow-up code after the @Panic in the test block can be run. Depends on ziglang#14152. Closes ziglang#1356.

This PR simplifies the setup code and handling of non-standard file descriptors and handles for Windows and Posix with main focus on pipes. Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle inhertance on Windows except shortly before and after the creation of the "child process". Leaking of file desriptors on Posix 1. CLOEXEC does not take immediately effect with clone(), but after the setup code for the child process was run and a exec* function is executed. External code may at worst be long living and never do exec*. 2. File descriptors without CLOEXEC are designed to "leak to the child", but every spawned process at the same time gets them as well. Leaking of handles on Windows 1. File leaking on Windows can be fixed with an explicit list approach as suggested in ziglang#14251, which might require runtime probing and allocation of the necessary process setup list. Otherwise, it might break on Kernel updates. 2. The potential time for leaking can be long due trying to spawn on multiple PATH and PATHEXT entries on Windows.

Panic test runner starts itself as child process with the test function index representing a test block. The custom panic handler compares a global previously written string. If the panic message is expected, the success count is increased and the next test block is run. On failure the parent process prints collected results, before it terminates. This means, that only one @Panic is possible within a test block and that no follow-up code after the @Panic in the test block can be run. Depends on ziglang#14152. Closes ziglang#1356.

andrewrk · 2023-10-19T00:08:00Z

It is unclear to me whether this was ever ready for review. Either way, it has succumbed to bitrot and must be refreshed in a new PR against the latest master. If you do refresh this in a new PR, please try to make it easier to review, clearly explaining the problem you are solving with a motivating use case, and don't leave open questions such as a comment on your own code that says "I dont like this solution.". Instead, do your best and then be clear that you want it to be merged.

matu3ba · 2023-10-19T13:31:04Z

For posterity: This contains 1. windows docs improvements for CreateProcess, 2. windows spawn process flags as I originally intended to use the advanced api, but did not use it due to external library compatibility concerns and 3. the code to unify posix and windows pipes.

Original purpose was to use it in the panic test runner, but I used instead #15991 to enable making things compatible with the server client approach.

matu3ba force-pushed the extra_streams branch 2 times, most recently from de0ab14 to 9bc166a Compare January 3, 2023 20:34

This comment was marked as resolved.

Sign in to view

matu3ba changed the title ~~wip std.child_process: enable non-standard streams~~ std.child_process: enable non-standard streams Jan 4, 2023

matu3ba marked this pull request as ready for review January 4, 2023 21:42

matu3ba commented Jan 4, 2023

View reviewed changes

test/standalone/childprocess_extrapipe/parent.zig Outdated Show resolved Hide resolved

matu3ba commented Jan 4, 2023

View reviewed changes

test/standalone/childprocess_extrapipe/parent.zig Outdated Show resolved Hide resolved

matu3ba commented Jan 4, 2023

View reviewed changes

lib/std/child_process.zig Outdated Show resolved Hide resolved

matu3ba marked this pull request as draft January 4, 2023 22:32

This comment was marked as resolved.

Sign in to view

matu3ba force-pushed the extra_streams branch 3 times, most recently from e0cd603 to b64cbee Compare January 6, 2023 19:44

This comment was marked as outdated.

Sign in to view

matu3ba force-pushed the extra_streams branch from 148ccdc to aff7844 Compare January 9, 2023 19:42

matu3ba commented Jan 9, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

matu3ba commented Jan 10, 2023

View reviewed changes

matu3ba force-pushed the extra_streams branch from 7513ddb to 83a15ca Compare January 10, 2023 23:33

matu3ba marked this pull request as ready for review January 10, 2023 23:42

matu3ba force-pushed the extra_streams branch from 83a15ca to 127651b Compare January 11, 2023 16:33

matu3ba force-pushed the extra_streams branch from ddee2dd to 207f3dd Compare January 17, 2023 19:42

andrewrk force-pushed the extra_streams branch from 207f3dd to f1df62b Compare January 27, 2023 06:07

matu3ba force-pushed the extra_streams branch from f1df62b to d824883 Compare February 4, 2023 19:48

matu3ba force-pushed the extra_streams branch from d824883 to 223fd9f Compare February 21, 2023 22:44

andrewrk closed this Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std.child_process: enable non-standard streams #14152

std.child_process: enable non-standard streams #14152

matu3ba commented Jan 1, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

matu3ba left a comment •

edited

Loading

This comment was marked as outdated.

matu3ba Jan 10, 2023 •

edited

Loading

matu3ba Jan 10, 2023

matu3ba commented Jan 10, 2023 •

edited

Loading

andrewrk commented Oct 19, 2023

matu3ba commented Oct 19, 2023

std.child_process: enable non-standard streams #14152

std.child_process: enable non-standard streams #14152

Conversation

matu3ba commented Jan 1, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

matu3ba left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

matu3ba Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

matu3ba Jan 10, 2023

Choose a reason for hiding this comment

matu3ba commented Jan 10, 2023 • edited Loading

andrewrk commented Oct 19, 2023

matu3ba commented Oct 19, 2023

matu3ba commented Jan 1, 2023 •

edited

Loading

matu3ba left a comment •

edited

Loading

matu3ba Jan 10, 2023 •

edited

Loading

matu3ba commented Jan 10, 2023 •

edited

Loading